Skip to content

Inject mem_mb into existing resources#514

Merged
victorlin merged 2 commits intomasterfrom
victorlin/snakemake-resources
Mar 25, 2026
Merged

Inject mem_mb into existing resources#514
victorlin merged 2 commits intomasterfrom
victorlin/snakemake-resources

Conversation

@victorlin
Copy link
Member

@victorlin victorlin commented Mar 20, 2026

Description of proposed changes

The trick with stack-walking --log-handler-script is a clever hack, but it relies on Snakemake's internal implementation.

Instead, parse the arguments for resources and either (1) update them in-place or (2) append the option at the end as done previously.

Related issue(s)

Addresses the removed TODO:

# XXX TODO: Support parsing of --resources to see if "mem_mb" is
# provided. If it's not, we could add our own "mem_mb" constraint
# alongside the other values of --resources. Punting on this
# because it's not as simple as appending an additional argument.
# So for now, if folks are specifying their own --resources,
# they'll also need to explicitly provide "mem_mb", which may mean
# repeating a previous --memory argument they provided us.
# -trs, 20 May 2020
#
# We might accomplish this TODO with a bit of a trick: using a
# stack-walking --log-handler-script to get access to
# Snakemake's in-process state and update --resources from
# there. I wrote a proof of concept¹ when exploring options
# around custom resources for an ncov PR², and it worked well
# in manual testing.
# -trs, 1 Feb 2023
#
# ¹ <https://gist.github.com/tsibley/6b3b5c37e651518d85810945a4140cde>
# ² <https://github.com/nextstrain/ncov/pull/1045>
warn(dedent("""
Warning: The explicit %s option passed to Snakemake prevents
the Nextstrain CLI from automatically providing a "mem_mb" resource
based on its --memory option. This may or may not be what you expect.
""" % (snakemake_opts["--resources"][0],)))

Checklist

  • Checks pass
  • Added doctests
  • Update changelog

Match Snakemake's own argument handling by treating -- as the end of
option parsing.
Copy link
Contributor

@joverlee521 joverlee521 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to tackle this longstanding TODO! I'm not sure --resources concurrent_deploys=2 is the way we want work around setdefault described in nextstrain/public#37, but good to have it as a fallback option.

The trick with stack-walking --log-handler-script is a clever hack, but
it relies on Snakemake's internal implementation.

Instead, parse the arguments for resources and either (1) update them
in-place or (2) append the option at the end as done previously.
@victorlin victorlin force-pushed the victorlin/snakemake-resources branch from c8486e8 to 8d1e6b2 Compare March 25, 2026 18:28
@victorlin victorlin merged commit fcd6d7a into master Mar 25, 2026
59 checks passed
@victorlin victorlin deleted the victorlin/snakemake-resources branch March 25, 2026 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants